fix(settings,wizard): resolve Closure rendering fatals and credits textarea [object Object]#1148
Conversation
… credits_custom_html [object Object] Fixes multiple rendering paths where callable/Closure values could reach esc_html(), esc_attr(), or wp_kses() and cause: "Object of class Closure could not be converted to string". Changes include: - Normalize wizard section display values before rendering (titles, descriptions, labels, booleans). - Harden setup default wizard template against callable/non-string values. - Expand Field attribute resolution to safely resolve callable attributes and nested callable html attrs. - Ensure class/wrapper class returns use resolved values. - Add defensive normalization in textarea field template. - Resolve callable defaults in settings value/display_value fallbacks. - Fix credits_custom_html default/value/display_value handling so textarea no longer shows "[object Object]". - Add fallback recovery for previously stored "[object Object]" value in credits_custom_html rendering. Validation: - PHP lint passed for all modified files: - inc/admin-pages/class-wizard-admin-page.php - views/wizards/setup/default.php - inc/ui/class-field.php - views/admin-pages/fields/field-textarea.php - inc/class-settings.php - inc/class-credits.php
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughResolve and normalize callable/default/template values across wizard sections, settings fields, UI Field attributes, credits, and templates so callables are invoked and non-stringable values are coerced to safe renderable forms before output. ChangesCallable Resolution & Display Normalization
Sequence Diagram(omitted — conditions for diagram generation not met) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
views/admin-pages/fields/field-textarea.php (2)
76-76: 💤 Low valuePrefer
esc_textarea()for textarea content.
esc_attr()works (both functions htmlspecialchars-encode), but WordPress's documented idiom for content rendered inside<textarea>...</textarea>isesc_textarea(). This was pre-existing, but since the surrounding line was modified for$value, it's a low-cost moment to align.♻️ Proposed change
- <textarea class="form-control wu-w-full wu-my-1 <?php echo esc_attr(trim($field_classes)); ?>" name="<?php echo esc_attr($field->id); ?>" placeholder="<?php echo esc_attr($placeholder); ?>" <?php $field->print_html_attributes(); ?>><?php echo esc_attr($value); ?></textarea> + <textarea class="form-control wu-w-full wu-my-1 <?php echo esc_attr(trim($field_classes)); ?>" name="<?php echo esc_attr($field->id); ?>" placeholder="<?php echo esc_attr($placeholder); ?>" <?php $field->print_html_attributes(); ?>><?php echo esc_textarea($value); ?></textarea>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@views/admin-pages/fields/field-textarea.php` at line 76, The textarea currently echoes the field value with esc_attr($value); update this to use WordPress's esc_textarea() for content inside <textarea> by replacing esc_attr($value) with esc_textarea($value) in the textarea element (keep the surrounding attributes and the call to $field->print_html_attributes() unchanged).
15-53: ⚡ Quick winSame four-block duplication as
views/wizards/setup/default.php.The "if callable, call; if scalar/__toString, cast; else ''" pattern is repeated four times here for
wrapper_classes,field_classes,placeholder, andvalue. Extracting a small helper makes the template easier to maintain and matches the suggestion on the wizard template. If both files ship with the same pattern, consider a sharedwu_*template helper instead of inlining in each view.♻️ Proposed refactor
-$wrapper_classes = $field->wrapper_classes; - -if (is_callable($wrapper_classes)) { - $wrapper_classes = call_user_func($wrapper_classes, $field); -} - -$wrapper_classes = is_scalar($wrapper_classes) || (is_object($wrapper_classes) && method_exists($wrapper_classes, '__toString')) - ? (string) $wrapper_classes - : ''; - -$field_classes = $field->classes; - -if (is_callable($field_classes)) { - $field_classes = call_user_func($field_classes, $field); -} -// ... +$resolve = static function ($value, $field): string { + if (is_callable($value)) { + $value = call_user_func($value, $field); + } + if (is_scalar($value) || (is_object($value) && method_exists($value, '__toString'))) { + return (string) $value; + } + return ''; +}; + +$wrapper_classes = $resolve($field->wrapper_classes, $field); +$field_classes = $resolve($field->classes, $field); +$placeholder = $resolve($field->placeholder, $field); +$value = $resolve($field->value, $field);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@views/admin-pages/fields/field-textarea.php` around lines 15 - 53, The template duplicates the "if callable then call; if scalar or __toString then cast else empty" pattern for $wrapper_classes, $field_classes, $placeholder, and $value; extract that logic into a small helper (e.g., wu_cast_to_string or similar) and replace the repeated blocks by calling that helper for each of those variables (call the helper with the original value and $field when callables need invocation). Ensure the helper handles callables, scalar/objects with __toString, and returns '' otherwise, and update views/wizards/setup/default.php to use the same helper (or create a shared template helper wu_* used by both views).inc/class-credits.php (2)
130-130: 💤 Low valueUnnecessary
function_exists('get_main_site_id')guard.
get_main_site_id()is a WordPress core function since 4.9.0, and per the project guidelines the plugin targets WordPress 5.3+, so this check will always succeed. Callingget_main_site_id()directly (and falling back if needed in non-multisite viais_multisite()instead) is clearer. Note the same pattern exists inbuild_custom_credit()at line 189 — likely worth aligning if you touch this.As per coding guidelines: "Code must work on WordPress 5.3+".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@inc/class-credits.php` at line 130, Replace the unnecessary function_exists('get_main_site_id') guard with a clear is_multisite() check: when is_multisite() is true, call get_site_url(get_main_site_id()) directly; otherwise fall back to network_home_url('/'). Update the same pattern in build_custom_credit() as well so both $url computations use is_multisite() and direct get_main_site_id() calls instead of function_exists().
132-137: ⚡ Quick winAwkward i18n: closing
</a>is hardcoded inside the translation string.The format string
'Powered by %1$s%2$s</a>'puts the opening tag in%1$s, the link text in%2$s, and bakes the closing</a>into the translatable text. Translators can easily break the markup by reordering, omitting, or rewriting around</a>, and translation tooling that escapes/normalizes HTML may corrupt the tag. The conventional WordPress pattern is to use symmetric placeholders for the opening and closing tags so the translation only contains visible text.♻️ Proposed fix using symmetric tag placeholders
return sprintf( - /* translators: 1: Opening anchor tag with URL to main site. 2: Network name. */ - __('Powered by %1$s%2$s</a>', 'ultimate-multisite'), - '<a href="' . esc_url($url) . '" target="_blank">', - esc_html($name) + /* translators: 1: Opening anchor tag, 2: Network name, 3: Closing anchor tag. */ + __('Powered by %1$s%2$s%3$s', 'ultimate-multisite'), + '<a href="' . esc_url($url) . '" target="_blank">', + esc_html($name), + '</a>' );As per coding guidelines: "All user-facing strings must use WordPress i18n functions ... with text domain
ultimate-multisite."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@inc/class-credits.php` around lines 132 - 137, The translation string currently hardcodes the closing </a> tag, which risks translators breaking markup; update the sprintf call in inc/class-credits.php so the closing tag is a separate placeholder (e.g. use three placeholders for opening tag, link text, and closing tag) and pass the arguments '<a href="' . esc_url($url) . '" target="_blank">', esc_html($name), and '</a>' (using the existing $url and $name variables) so the translatable string contains only visible text and symmetric tag placeholders.inc/ui/class-field.php (1)
339-372: 💤 Low valueResolver helpers look good; one note on the new callable-arg contract.
is_resolvable_callable()correctly limits callable resolution towu_get_*functions, method-array callables, and Closures — that defensive narrowing avoids accidentally invoking arbitrary strings that happen to be valid function names (e.g.'phpinfo'as an attribute value). The recursiveresolve_attribute_value()then walks nested arrays and stringifies__toString()objects.One subtle behavioral change:
call_user_func($attr, $this)now passes the field instance as an argument to every resolved callable. Previously these callables were invoked with no args. PHP tolerates extra args silently for closures/methods that don't declare them, so existing zero-arg closures (e.g. the new ones ininc/class-credits.phpandinc/class-settings.php) keep working — but if you intend this as a documented contract, please add a@param self $fieldnote in the docblock so addons can rely on it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@inc/ui/class-field.php` around lines 339 - 372, The resolver now invokes callables with the field instance (call_user_func($attr, $this)) which changes the callable contract; update the docblock for resolve_attribute_value() to document this by adding a `@param` self $field (or similar) note indicating that resolved callables will receive the current field instance as their first argument, and mention is_resolvable_callable() so callers know which callables may be invoked with that argument.views/wizards/setup/default.php (1)
21-79: ⚡ Quick winHeavy duplication — six near-identical normalization blocks.
The same six-line "resolve callable, coerce scalar/__toString, fallback to ''" pattern is repeated for
title,description,content,back_label,skip_label, andnext_label. A small inline helper or a single loop over a key→value map would reduce ~60 lines to a single block and make future edits a one-place change.♻️ Proposed refactor using a tiny inline helper
-$title_value = ''; - -if (is_callable($title)) { - $title = call_user_func($title); -} - -if (null !== $title && (is_scalar($title) || (is_object($title) && method_exists($title, '__toString')))) { - $title_value = (string) $title; -} - -$description_value = ''; - -if (is_callable($description)) { - $description = call_user_func($description); -} - -if (null !== $description && (is_scalar($description) || (is_object($description) && method_exists($description, '__toString')))) { - $description_value = (string) $description; -} - -// ... 4 more identical blocks ... +$wu_resolve = static function ($value): string { + if (is_callable($value)) { + $value = call_user_func($value); + } + if (is_scalar($value) || (is_object($value) && method_exists($value, '__toString'))) { + return (string) $value; + } + return ''; +}; + +$title_value = $wu_resolve($title); +$description_value = $wu_resolve($description); +$content_value = $wu_resolve($content); +$back_label_value = $wu_resolve($back_label); +$skip_label_value = $wu_resolve($skip_label); +$next_label_value = $wu_resolve($next_label);Note that this normalization is also a second pass —
Wizard_Admin_Page::prepare_section_for_display()already resolves these values before the template runs. The defensive double-resolution is fine, but worth being aware of when reviewing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@views/wizards/setup/default.php` around lines 21 - 79, The repeated normalization blocks for title, description, content, back_label, skip_label, and next_label should be replaced by a single helper/closure (e.g., normalize_display_value) that: accepts a nullable mixed, calls it if is_callable, then returns a string if scalar or object with __toString or '' otherwise; then call this helper for each variable (title, description, content, back_label, skip_label, next_label) to set the corresponding *_value variables; reference existing symbols like $title, $description, $content, $back_label, $skip_label, $next_label and note that Wizard_Admin_Page::prepare_section_for_display() already does similar work so keep this as a defensive second pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@inc/ui/class-field.php`:
- Line 67: The class declaration was accidentally renamed from class Field to
class lField causing autoload, instanceof checks, and all consumers (e.g.,
usages in class-form.php, class-settings.php, class-settings-endpoint.php,
class-settings-admin-page.php, views/admin-pages/fields/field-textarea.php and
tests WP_Ultimo/UI/Field_Test.php) to break; restore the original class name by
renaming the declaration back to "class Field implements \JsonSerializable" in
the file that currently defines lField (class-field.php) so it matches the
project autoload convention and existing references, or if a temporary
compatibility layer is required add a
class_alias('WP_Ultimo\\UI\\Field','WP_Ultimo\\UI\\lField') after the correct
class declaration—prefer simply reverting to "Field".
---
Nitpick comments:
In `@inc/class-credits.php`:
- Line 130: Replace the unnecessary function_exists('get_main_site_id') guard
with a clear is_multisite() check: when is_multisite() is true, call
get_site_url(get_main_site_id()) directly; otherwise fall back to
network_home_url('/'). Update the same pattern in build_custom_credit() as well
so both $url computations use is_multisite() and direct get_main_site_id() calls
instead of function_exists().
- Around line 132-137: The translation string currently hardcodes the closing
</a> tag, which risks translators breaking markup; update the sprintf call in
inc/class-credits.php so the closing tag is a separate placeholder (e.g. use
three placeholders for opening tag, link text, and closing tag) and pass the
arguments '<a href="' . esc_url($url) . '" target="_blank">', esc_html($name),
and '</a>' (using the existing $url and $name variables) so the translatable
string contains only visible text and symmetric tag placeholders.
In `@inc/ui/class-field.php`:
- Around line 339-372: The resolver now invokes callables with the field
instance (call_user_func($attr, $this)) which changes the callable contract;
update the docblock for resolve_attribute_value() to document this by adding a
`@param` self $field (or similar) note indicating that resolved callables will
receive the current field instance as their first argument, and mention
is_resolvable_callable() so callers know which callables may be invoked with
that argument.
In `@views/admin-pages/fields/field-textarea.php`:
- Line 76: The textarea currently echoes the field value with esc_attr($value);
update this to use WordPress's esc_textarea() for content inside <textarea> by
replacing esc_attr($value) with esc_textarea($value) in the textarea element
(keep the surrounding attributes and the call to $field->print_html_attributes()
unchanged).
- Around line 15-53: The template duplicates the "if callable then call; if
scalar or __toString then cast else empty" pattern for $wrapper_classes,
$field_classes, $placeholder, and $value; extract that logic into a small helper
(e.g., wu_cast_to_string or similar) and replace the repeated blocks by calling
that helper for each of those variables (call the helper with the original value
and $field when callables need invocation). Ensure the helper handles callables,
scalar/objects with __toString, and returns '' otherwise, and update
views/wizards/setup/default.php to use the same helper (or create a shared
template helper wu_* used by both views).
In `@views/wizards/setup/default.php`:
- Around line 21-79: The repeated normalization blocks for title, description,
content, back_label, skip_label, and next_label should be replaced by a single
helper/closure (e.g., normalize_display_value) that: accepts a nullable mixed,
calls it if is_callable, then returns a string if scalar or object with
__toString or '' otherwise; then call this helper for each variable (title,
description, content, back_label, skip_label, next_label) to set the
corresponding *_value variables; reference existing symbols like $title,
$description, $content, $back_label, $skip_label, $next_label and note that
Wizard_Admin_Page::prepare_section_for_display() already does similar work so
keep this as a defensive second pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 418c27ea-9a28-41a3-9842-1ffdbce1cc30
📒 Files selected for processing (6)
inc/admin-pages/class-wizard-admin-page.phpinc/class-credits.phpinc/class-settings.phpinc/ui/class-field.phpviews/admin-pages/fields/field-textarea.phpviews/wizards/setup/default.php
superdav42
left a comment
superdav42
left a comment
There was a problem hiding this comment.
LGTM!
Thanks for your contribution.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@inc/class-credits.php`:
- Around line 103-113: The current corruption fallback only normalizes '[object
Object]' inside the field state closures; update build_credit_html (and any
runtime rendering path in class-credits.php that reads
wu_get_setting('credits_custom_html')) to apply the same normalization: when
wu_get_setting('credits_custom_html', ...) returns the literal '[object Object]'
(trimmed) replace it with $this->get_default_custom_credit_html() before
rendering, or persist the fixed value back via the same setter so footer output
never prints the corrupted token; reference build_credit_html,
get_default_custom_credit_html, and the wu_get_setting('credits_custom_html',
...) call to locate and change the runtime read/replace logic.
In `@inc/ui/class-field.php`:
- Around line 300-305: The code assumes $this->atts['wrapper_html_attr'] is an
array when checking $this->atts['wrapper_html_attr']['v-show'] inside the branch
for ('wrapper_classes' === $att); guard or resolve callable values first by
checking if $this->atts['wrapper_html_attr'] is callable (is_callable) and
invoking it to obtain the actual value, or ensure it is an array (is_array)
before accessing ['v-show']; update the logic around the wrapper_html_attr
lookup used with 'v-show' so it safely handles a Closure/callable and only
performs ['v-show'] array access after resolving to an array, keeping the
existing behavior of appending ' wu-requires-other' to $attr when v-show is
present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ccf96c91-f836-4a2c-bdc7-e196248753f9
📒 Files selected for processing (5)
inc/class-credits.phpinc/functions/template.phpinc/ui/class-field.phpviews/admin-pages/fields/field-textarea.phpviews/wizards/setup/default.php
✅ Files skipped from review due to trivial changes (1)
- inc/functions/template.php
🚧 Files skipped from review as they are similar to previous changes (1)
- views/admin-pages/fields/field-textarea.php
Resolve `wrapper_html_attr` through `resolve_attribute_value()` before checking for a `v-show` key to prevent a fatal when the attribute is registered as a Closure.
…endering Extracted repeated corruption-guard logic into normalize_custom_credit_html() and applied it in build_credit_html() so footer output never prints the corrupted token even when the setting was stored before the Closure-leak fix.
2 participants
Fixes #1144
Summary
This PR fixes multiple rendering issues caused by callable/closure values being passed into escaped output without being resolved first.
What was fixed
Files changed
wp-content/plugins/ultimate-multisite/inc/admin-pages/class-wizard-admin-page.php
wp-content/plugins/ultimate-multisite/views/wizards/setup/default.php
wp-content/plugins/ultimate-multisite/inc/ui/class-field.php
wp-content/plugins/ultimate-multisite/views/admin-pages/fields/field-textarea.php
wp-content/plugins/ultimate-multisite/inc/class-settings.php
wp-content/plugins/ultimate-multisite/inc/class-credits.php
Behavior impact
Validation performed
PHP lint checks passed for all modified files:
Summary by CodeRabbit
Bug Fixes
Improvements
Chores